Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

Add more type safety to datastore get #420

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add more type safety to datastore get #420

wants to merge 1 commit into from

Conversation

saulshanabrook
Copy link
Contributor

@saulshanabrook saulshanabrook commented Aug 26, 2019 •
edited
Loading

This PR makes the datastore class generic with respect to the schemas it was created with. This means that the get method now can verify that the schema you pass is of the same type as the schemas you created the store with.

Note that this should produce no change in emitted code, only in the type declarations.

For example, in the code below, d.get(s2) will fail, because it has a different type than s. We need the <const> type modifier so it types each id as the constant string instead of just a string.

const s = <const>{
  id: "dfd",
  fields: {
    test: new ListField()
  }
};

const s2 = <const>{
  id: "dfd1",
  fields: {
    test: new ListField()
  }
};

const d = Datastore.create({ schemas: [s], id: 0 });

d.get(s2);
d.get(s);

Screen Shot 2019-08-26 at 8 38 35 AM

@vidartf I think you mentioned wanting something like this in one of our earlier meetings, if I am not mistaken.

cc @ian-r-rose @jasongrout

See microsoft/TypeScript#20965 for a discussion of how to get the type of an array, using T[number]

Original ideas was posted here jupyterlab/jupyterlab#5382 (comment)

@saulshanabrook
Make. Datastore generic with schema types
082a697 
This increases the type safety of the get table function, so that
only registered schemas can get retrieved.
@saulshanabrook
Copy link
Contributor Author

I also am having a go at passing in a schemas object of { readonly [id: string]: { readonly [name: string]: AnyField } } instead of an array of schemas into the Datastore.create method. That way, we could have type safe get access` where you just pass in the id, instead of the whole schema. However, I am blocked by microsoft/TypeScript#33084

@vidartf
Copy link
Contributor

vidartf commented Aug 26, 2019

I also am having a go at passing in a schemas object

This is what I was thinking of previously, but didn't end up researching it. The issue you mentioned was resolved, so does that mean it is possible?

@saulshanabrook
Copy link
Contributor Author

@vidartf yeah it would just require some larger code changes to the datastores API. Does this PR get you the safety you were looking for?

vidartf
vidartf reviewed Aug 27, 2019
View reviewed changes
Copy link
Contributor

@vidartf vidartf left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look at this, and I think the issue with T[number] is that it creates a union type of the schemas in unfortunate ways. E.g. the change type ends up being:

{ [schemaId: string]: <union type of all schema change types> }

This means the type checking in any change handlers is confused without a cast. I started playing around with some alternative approaches (and ended up spending too much time, but I learned a lot about advanced types), and ended up making a PR to @saulshanabrook's branch: saulshanabrook#1

@saulshanabrook
Copy link
Contributor Author

I think the issue with T[number] is that it creates a union type of the schemas in unfortunate ways

That's what I would expect. What would you expect the change type to be? A union seems helpful because then in the changed signal you can use branching to handle the different tables and it will be type safe.

@saulshanabrook saulshanabrook mentioned this pull request Nov 13, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vidartf vidartf vidartf left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@saulshanabrook @vidartf